-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
B 20939 int #13538
B 20939 int #13538
Conversation
} | ||
} | ||
} | ||
return "Uncertified" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 418 and 434: I'm assuming this is a worse case scenario? It should never get to this, right? You may want to throw an error and/or log here.
In addition, you want to create a generic method in place of formatAOADate/formatSSWDate methods that takes in the cert array, uuid and enum. It should return null if nothing matches. From there you can log or throw an error in the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
418 is what we discussed in the standup this morning. Right now, there's a way to download the AOA before the SC submits move details where their signature is added. Until we can revisit the aoa download availability, our PO just wants it to use the date the PDF generation was ran.
As for 434, this should never occur, so an error may be more appropriate here. If that's what we want to go with, I'll add it to the todo list for later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone ahead and added the error handling to this method. Now, if there's ever an uncertified payment packet downloaded before it should be, it will error out. Honestly, making this sort of thing easier in this service was long overdue, so I think that's a great change.
As for the generic method idea, I'm going to push back against that because that's the solution I attempted initially. Due to the implementation in the way that the service determines which packet is being generated, as well as the best way for handling adding the dates themselves from the certs, and the two alternate solutions to when a date shouldn't be used... I believe the current solution is easier to read, easier to maintain, is marginally better performance-wise, and uses about the same amount of code as combining them.
I do realize that the amount of methods in this service is... kind of a lot, so I'd see why we'd want to cut down on them. But if I need more justification still, then an additional reason for adding 2 in this story is that these changes also removed a near-duplicate method in formatSignatureDate.
Just let me know if any objections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
B-20939
Summary
This task adds in the preparation date field for the SSW in the top right hand corner, and includes some other miscellaneous organization changes and test fixes to make the service easier to work with for upcoming features.
How to test
Backend
Screenshots